Skip to content

sandboxes: follow-up fixes and clarifications from sbx docs rewrite#24591

Merged
dvdksn merged 7 commits intodocker:mainfrom
dvdksn:sbx-follow-up-fixes
Mar 31, 2026
Merged

sandboxes: follow-up fixes and clarifications from sbx docs rewrite#24591
dvdksn merged 7 commits intodocker:mainfrom
dvdksn:sbx-follow-up-fixes

Conversation

@dvdksn
Copy link
Copy Markdown
Contributor

@dvdksn dvdksn commented Mar 31, 2026

Summary

Follow-up to #24590 (sbx docs rewrite):

  • Fix broken anchor link in `troubleshooting.md`: `#monitoring-network-activity` → `#monitoring`
  • Fix incorrect Windows Docker daemon limitation on `docker-desktop.md` (doesn't apply to legacy `docker sandbox` command)
  • Address bot review feedback: missing `mkdir -p` prerequisite, vague feature description, misleading shell-sourcing instruction, unclear "elevated permissions" wording
  • Clarify `-docker` variants run inside the microVM with no local Docker daemon dependency; fix ambiguous "They" pronoun
  • Update `check-pr` skill to reply to and resolve review comments on merged PRs

Learnings

  • When bot review comments are acknowledged as valid, they should be fixed in the same session — not deferred. "Candidate for follow-up" without a tracking issue is just noise. Evaluate each comment: fix it or dismiss it with a reason.

Generated by Claude Code

The link to the Monitoring section in policy.md used the wrong anchor
(#monitoring-network-activity). The actual heading generates #monitoring.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@dvdksn dvdksn added the status/review Pull requests that are ready for review label Mar 31, 2026
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 31, 2026

Deploy Preview for docsdocker ready!

Name Link
🔨 Latest commit 363863b
🔍 Latest deploy log https://app.netlify.com/projects/docsdocker/deploys/69cbfcdceaeffa00081861d3
😎 Deploy Preview https://deploy-preview-24591--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@dvdksn dvdksn requested a review from a team March 31, 2026 16:26
dvdksn and others added 4 commits March 31, 2026 18:30
Step 2 (merged) previously stopped immediately with "no further action
needed", skipping all comment handling. Now it checks for unanswered
review comments and replies to them. Also adds GraphQL thread-resolution
to step 5 so threads get marked resolved after being addressed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- troubleshooting: add mkdir -p ./result prerequisite before tar extract
- docker-desktop: replace vague "limited features" with direct pointer to sbx CLI
- docker-desktop: clarify restart (not source) is what matters for daemon env vars
- docker-desktop: promote Windows Docker daemon caveat to NOTE callout
- get-started: replace "elevated permissions" with explicit "Run as Administrator"

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ktop page

The docker sandbox (legacy) command doesn't have the Docker-not-included
limitation on Windows — that only applies to the sbx CLI. Remove the
incorrect NOTE callout.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Make clear no local Docker daemon is needed
- Disambiguate "They" — replace with "These variants"
- Note that privileged mode applies inside the microVM boundary, not the host

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@dvdksn dvdksn changed the title sandboxes: fix broken anchor link in troubleshooting sandboxes: follow-up fixes and clarifications from sbx docs rewrite Mar 31, 2026
@dvdksn
Copy link
Copy Markdown
Contributor Author

dvdksn commented Mar 31, 2026

/review

Restarting Docker Desktop is what causes the daemon to pick up the new
env var. Opening a new terminal already gives the user the variable.
Sourcing explicitly adds nothing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reverts the unnecessary change to the sourcing step.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@akristen akristen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@dvdksn dvdksn merged commit 04f9e20 into docker:main Mar 31, 2026
14 checks passed
@dvdksn dvdksn deleted the sbx-follow-up-fixes branch March 31, 2026 17:01
Copy link
Copy Markdown
Contributor

@docker-agent docker-agent Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

This follow-up PR correctly addresses the bot review feedback from #24590. All the fixes are appropriate:

Anchor link fixed: #monitoring-network-activity#monitoring in troubleshooting.md
Prerequisites clarified: Added "Run as Administrator" instruction for Windows Hypervisor Platform setup
Missing step added: Added mkdir -p ./result prerequisite in troubleshooting example
Privileged mode clarified: Specified that -docker variants run in privileged mode "inside the microVM (not on your host)"
Shell sourcing improved: Clarified that sourcing is optional for current terminal, required action is restarting Docker Desktop
Windows limitation corrected: Removed incorrect statement about Docker daemon not being available in docker sandbox command

The changes follow the style guide, improve clarity, and don't introduce any new issues. Nice work addressing the feedback systematically!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/review Pull requests that are ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants